cache compiled schema and implement cache invalidation strategy#16
cache compiled schema and implement cache invalidation strategy#16srivastava-diya wants to merge 11 commits into
Conversation
jdesrosiers
left a comment
jdesrosiers
left a comment
There was a problem hiding this comment.
We need to handle cache invalidation when a schema changes.
srivastava-diya
force-pushed
the
feature/schema-caching
branch
from
0ad2b54 to
46a1d63
Compare
srivastava-diya
changed the title
id to fixtureSchemaUri in schemaValidation test
use json-document model
srivastava-diya
force-pushed
the
feature/schema-caching
branch
from
006ca38 to
58f499e
Compare
|
hey @jdesrosiers This PR is still a WIP, so I'm not looking for approval just yet. I'd really appreciate a review to make sure if I'm proceeding in the right direction before I continue implementing the other stuff. |
jdesrosiers
left a comment
jdesrosiers
left a comment
There was a problem hiding this comment.
You're on the right track! There's some clean up and optimization we can do, but we can focus on that later after we get it working.
|
The PR ended up getting pretty big, so I'd appreciate a review whenever it's convenient for you. |
| const dependentSchemaUris = this.schemaStore.getDependentSchemaUris(this.schemaUri); | ||
|
|
||
| if (dependentSchemaUris === undefined) { | ||
| return true; |
There was a problem hiding this comment.
i added true here so the document call validateSchema(), which compiles the latest schema version from disk and dependency cache can be filled.
There was a problem hiding this comment.
This doesn't make sense to me. When the document is opened, it gets validated and the cache is filled. When a file changes, the only reason the dependent schemas should be undefined is if there's no schemaUri for the document. If that's the case, the document isn't dependent on any schemas and it should return false.
If I'm missing something, the best way to help me understand is to write a test that demonstrates the problem.
| } | ||
|
|
||
| private validate() { | ||
| this.parseErrors = []; |
There was a problem hiding this comment.
This isn't resolved. The second case I mentioned appears to be handled, but not the first case. Write tests for both cases to make sure.
| const toClear: string[] = []; | ||
| for (const [schemaUri, compiledSchema] of this.compiledSchemaCache.entries()) { | ||
| const dependentSchemas = this.getDepsFromCompiledSchema(compiledSchema); | ||
| for (const uri of changedUris) { | ||
| if (dependentSchemas.has(uri)) { | ||
| toClear.push(schemaUri); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for (const schemaUri of toClear) { | ||
| this.clear(schemaUri); | ||
| } |
There was a problem hiding this comment.
Why collect URIs to be cleared instead of just calling clear then?
| const dependentSchemaUris = this.schemaStore.getDependentSchemaUris(this.schemaUri); | ||
|
|
||
| if (dependentSchemaUris === undefined) { | ||
| return true; |
There was a problem hiding this comment.
This doesn't make sense to me. When the document is opened, it gets validated and the cache is filled. When a file changes, the only reason the dependent schemas should be undefined is if there's no schemaUri for the document. If that's the case, the document isn't dependent on any schemas and it should return false.
If I'm missing something, the best way to help me understand is to write a test that demonstrates the problem.
2 participants
Description
The
validate(schemaUri)function is curried calling it with just the schema URI compiles the schema and returns a reusable validator function. Previously this compilation was happening on every keystroke.